feat: add /maps/ routes - #2
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This comment was marked as resolved.
This comment was marked as resolved.
|
Some windows failures around parallel uploads, which I suspect are probably a test artifact rather than a bug to be worried about. |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
A couple of small cleanup things to address but otherwise functionality seems thorough!
Out of curiosity, will I still need the patches I have on desktop related to CORS issues that I run into in development? (caused by usage of Vite's dev server)
Slightly tangential, but the publishing tsconfig needs to be updated since you're committing to a TS implementation:
diff --git a/tsconfig.npm.json b/tsconfig.npm.json
index f89cb7b..f35d2b4 100644
--- a/tsconfig.npm.json
+++ b/tsconfig.npm.json
@@ -4,9 +4,7 @@
"noEmit": false,
"outDir": "dist",
"declaration": true,
- "emitDeclarationOnly": true,
"declarationMap": true
},
- "files": [],
- "include": ["src/index.js"]
+ "include": ["src/index.ts", "types/*"]
}
Updates the bounding box used for the map where there's no data to show. Something I learned via digidem/comapeo-map-server#2 (comment)
Updates the bounding box used for the map where there's no data to show. Something I learned via digidem/comapeo-map-server#2 (comment)
achou11
left a comment
achou11
left a comment
There was a problem hiding this comment.
A few non-blocking comments but overall looks great ✨
| if (request.params.mapId !== CUSTOM_MAP_ID) { | ||
| throw new errors.MAP_NOT_FOUND(`Map not found: ${request.params.mapId}`) | ||
| } |
There was a problem hiding this comment.
feels misleading to use a not found error here. maybe a status error explaining this temporary (?) limitation would be more suitable?
There was a problem hiding this comment.
Fix is lined up in #4, which adds a FORBIDDEN (403) for trying to update or delete the "special" IDs - fallback and default. I'm following a pattern here where PUT is for updates, not for creation (keeping POST for that), so I think 404 is the correct response. This is probably not perfectly semantically correct, given that this is also creating the custom map, but I think we can just resolve this later when we change this to support multiple custom maps.
| if (request.params.mapId !== CUSTOM_MAP_ID) { | ||
| throw new errors.MAP_NOT_FOUND(`Map not found: ${request.params.mapId}`) | ||
| } |
There was a problem hiding this comment.
similar comment about potentially using a different error code
| // @ts-expect-error - the types for this are too hard and making them work would not add any type safety. | ||
| remoteDeviceId: z32.encode(req.socket.remotePublicKey), |
There was a problem hiding this comment.
More specific placement to avoid unintentional opt-out:
| // @ts-expect-error - the types for this are too hard and making them work would not add any type safety. | |
| remoteDeviceId: z32.encode(req.socket.remotePublicKey), | |
| remoteDeviceId: z32.encode( | |
| // @ts-expect-error - the types for this are too hard and making them work would not add any type safety. | |
| req.socket.remotePublicKey, | |
| ), |
3 participants
Adds routes for reading maps, uploading new custom maps, and deleting custom maps